Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics API scaler to support parsing numbers in quotes as well as quantities. #1668

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

devjoes
Copy link
Contributor

@devjoes devjoes commented Mar 12, 2021

Quantities can now be parsed from a metrics API response. Lots of metrics APIs return Quantity types which get marshalled to strings (KEDA itself actually has this behaviour).
ExternalMetricValue.Value is already a Quantity so there should be no impact on the wider codebase.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • [N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • [N/A] A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Fixes #1667

@ghost ghost force-pushed the main branch 2 times, most recently from 306226f to c5de4c2 Compare March 12, 2021 16:28
Quantity is returned as a member of ExternalMetricValue so there should be no impact.

Signed-off-by: Joe Shearn <joseph.shearn@sainsburys.co.uk>
@zroubalik
Copy link
Member

@turbaszek would you mind looking at this?

@zroubalik zroubalik requested a review from tomkerkhove March 15, 2021 09:01
r := gjson.GetBytes(body, valueLocation)
if r.Type == gjson.String {
q, err := resource.ParseQuantity(r.String())
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be returned if the err!=nil?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would revert to the original behaviour and return "valueLocation must point to value of type number got: string"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've improved the error handling

Copy link
Contributor

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look reasonable to me, just one question

@zroubalik
Copy link
Member

@devjoes could you please open another PR with documentation of this feature? in content/docs/2.2 https://github.com/kedacore/keda-docs

@ghost
Copy link

ghost commented Mar 16, 2021

@zroubalik docs updated

Signed-off-by: Joe Shearn <joseph.shearn@sainsburys.co.uk>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@zroubalik zroubalik merged commit 50b9090 into kedacore:main Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics API scaler does not support Quantities
3 participants